lib/src/reboot: use systemd-run to execute the reboot #1398
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The code changes modify the reboot function to use systemd-run for executing the reboot command. This ensures that systemd has the correct view into the mounted /run/nextroot directory, which is necessary for soft-reboots. I suggested adding a message to the systemd-run command to improve logging and debugging.
| Task::new("Rebooting system", "systemd-run") | ||
| .args(["reboot"]) | ||
| .run()?; |
There was a problem hiding this comment.
Consider adding a --message argument to systemd-run to provide a user-friendly message about the reboot, which can be displayed in system logs or by system monitoring tools. This can aid in debugging and understanding the reason for the reboot, especially in automated environments.
| Task::new("Rebooting system", "systemd-run") | |
| .args(["reboot"]) | |
| .run()?; | |
| .args(["--message=Rebooting system", "reboot"]) | |
| .run()?; |
There was a problem hiding this comment.
Huh cool that it knows about this! It's unrelated to this change, but we might as well do so. How about --message=Initiated by bootc?
There was a problem hiding this comment.
It would be cool if that was a valid argument 😆
Not sure where Gemini got this from, but I can't find it in any manpages.
There was a problem hiding this comment.
I wanted to followup on this specifically: Gemini didn't hallucinate in that sense, --message does exist as an argument...but for systemctl. It did mess up the patch. And the two humans here messed up in not double checking it.
In my case, I incorrectly believed we had CI covering this (which is totally my fault again!). Also, it's kind of subtle but visually when I was glancing at the PR, the removed lines have reboot and the added lines have --message and so (again at a super quick glance) it read to me like "reboot --message".
Finally and most importantly: I myself a while ago added a similar patch in the OpenShift MCO (though ironically actually, using --description for systemd-run now that I look...) but I did know that --message existed.
There was a problem hiding this comment.
man... I kept thinking how did this work for me locally when I saw the fallout. I did test what I originally submitted but never tested with --message
|
CI here is https://gitlab.com/redhat/centos-stream/containers/bootc/-/issues/1174 biting us - again skew between containers and rpms (cc rpm-software-management/dnf5#833 ) |
When we do a reboot it is triggered inside the bootc namespace. As we implement support for soft-reboots we need to make sure that systemd has a view into the mounted /run/nextroot to be able to act on doing a soft-reboot or a reboot. By using systemd-run we avoid the limited view in the current namespace. Signed-off-by: Joseph Marrero Corchado <[email protected]>
3 participants
When we do a reboot it is triggered inside the bootc namespace. As we implement support for soft-reboots we need to make sure that systemd has a view into the mounted /run/nextroot to be able to act on doing a soft-reboot or a reboot. By using systemd-run we avoid the limited view in the current namespace.